Skip to content

=tck #384 dont check for cause message when checking 3.9 #385

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 11, 2017
Merged

=tck #384 dont check for cause message when checking 3.9 #385

merged 1 commit into from
Jul 11, 2017
+84 −7

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Jul 4, 2017

Resolves #384 in the TCK, existing impls will remain to pass since they were already referring to 3.9

@ktoso ktoso mentioned this pull request Jul 4, 2017
Merged
@akarnokd
Copy link
Contributor

akarnokd commented Jul 4, 2017

Please don't forget the TCK explanations:

PublisherVerificationRules.java#L474

PublisherVerificationRules.java#L499

@ktoso
Copy link
Contributor Author

ktoso commented Jul 4, 2017

Thanks for the reminder @akarnokd !

PR currently blocked by decision on: #386 (comment) if we actually want to check the message at all or not - perhaps not.

Then I'll amend this PR and fix the comments too :)

@ktoso ktoso changed the title =tck #384 check for alternative wordings in messages when checking 3.9 =tck #384 dont check for cause message when checking 3.9 Jul 5, 2017
@ktoso
Copy link
Contributor Author

ktoso commented Jul 5, 2017

Aligned with spec wording proposed in #386 now.

Changes are:

  • we don't require any message content to pass the 3.9 rule
    • only that an IllegalArgumentException is signaled is checked (this is compatible with old impls and JDK9)
  • added an optional test to check if an impl actually references 3.9 or uses the proposed (in the spec as SHOULD) wording of "non-positive subscription request" (JDK wording) or the previous "3.9" reference which used to be required
    • this is compatible too, since it's just optional (and existing impls pass it by default since they passed the previous impl of the test)
  • amended explanations of these tests - thanks for the reminder David

@ktoso ktoso force-pushed the loosened-up-req-3.9-wording-ktoso branch from 85362eb to f8d203d Compare July 5, 2017 02:46
@viktorklang
Copy link
Contributor

LGTM.

@reactive-streams/contributors Please chime in so we can include this change in the RC2 ASAP. Thank you!

@ktoso ktoso mentioned this pull request Jul 7, 2017
Closed
@viktorklang
Copy link
Contributor

@reactive-streams/contributors 1 day left to comment on this!

@viktorklang
Copy link
Contributor

@reactive-streams/contributors Merging.

@viktorklang viktorklang merged commit 797f012 into reactive-streams:master Jul 11, 2017
@viktorklang viktorklang added this to the 1.0.1 milestone Jul 11, 2017
@ktoso ktoso deleted the loosened-up-req-3.9-wording-ktoso branch July 11, 2017 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akarnokd akarnokd akarnokd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.1
Development

Successfully merging this pull request may close these issues.
3 participants
@ktoso @akarnokd @viktorklang